Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

On macOS build universal arm64 and x86_64 #1214

Merged
merged 1 commit into from
Feb 11, 2022

Conversation

jminor
Copy link
Collaborator

@jminor jminor commented Feb 11, 2022

On macOS, we now build libopentime and libopentimelineio as universal (both arm64 and x86_64)

@JeanChristopheMorinPerso
Copy link
Member

Nice! FYI, you will have to modify the CI to output universal2 tagged wheels. See https://cibuildwheel.readthedocs.io/en/1.x/options/#archs. This can be done here: https://github.com/PixarAnimationStudios/OpenTimelineIO/blob/main/.github/workflows/python-package.yml#L117.

I just don't know if it's going to work for Python less than 3.8.

@meshula
Copy link
Collaborator

meshula commented Feb 11, 2022

@JeanChristopheMorinPerso I assume you mean, after this change lands, we can update the CI for universal tagged builds?

@meshula
Copy link
Collaborator

meshula commented Feb 11, 2022

Also, why 3.8 specifically? 3.7.x was when M1 support appeared?

@JeanChristopheMorinPerso
Copy link
Member

I guess wheel support could be added later...

As for why 3.8+, see https://cibuildwheel.readthedocs.io/en/stable/faq/#universal2

More specifically:

Only CPython 3.8 and newer support universal2 and arm64 wheels.

But the whole section is worth a read.

@JeanChristopheMorinPerso
Copy link
Member

I guess wheel support could be added later...

The triple dots were because I wasn't sure of the end result of having universal libs while having "non aware" wheels. But thinking about it more, all the tests are passing on all platforms and python versions. So everything will be fine.

Also I realize the tone of my initial comment kind of sounded like an order or authoritive. I'm really sorry for that. Thanks @meshula for pointing it out (kind of indirectly, but still).

The idea behind my comment was more to remind that it's something we shouldn't forget to do before the next release.

@meshula
Copy link
Collaborator

meshula commented Feb 11, 2022

No need to be sorry, there wasn't a tone problem, I was just looking for clarification :) I wanted to know if (1) we can land this change and it won't disrupt wheels (I understand from your comment that it won't) and (2) if the update to the wheel CI can be a second change (I understand from your comment that we can do it as a second step).

@meshula meshula merged commit 71aa2fa into AcademySoftwareFoundation:main Feb 11, 2022
@ssteinbach ssteinbach added this to the Public Beta 15 milestone Sep 19, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants